Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implementation scaling mode dynamic android and ios #270

Merged
merged 9 commits into from
Oct 17, 2023

Conversation

jonathanm-tkf
Copy link
Contributor

@jonathanm-tkf jonathanm-tkf commented Sep 26, 2023

Description

Currently, we don't have the possibility to change the Scaling Mode like on other platforms, similar to the fullscreen button.

Changes

     <PlayerView
        scalingMode={scalingMode}
      />

@jonathanm-tkf jonathanm-tkf changed the title chore: initial implementation android WIP: initial implementation android Sep 26, 2023
@jonathanm-tkf jonathanm-tkf changed the title WIP: initial implementation android WIP: initial implementation scaling mode dynamic android Sep 26, 2023
Copy link
Contributor

@krocard krocard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your pull request. After careful consideration, we believe the proposed approach of mutating the player's sytyleConfig is not desirable:

  • changes will not be taken into account by the player (player events will not be triggered)
  • the scaling mode is a property of the PlayerView, not the Player
  • React Native doesn't allow UI side-effects in @ReactMethod, UIManager has to be used instead.

Here follows our recommendation to implement PlayerView's ScalingMode. As the scaling mode property is similar to the fullscreen one, we have used it as an example for each step.

  1. Add a scalingMode parameter to the PlayerView function and PlayerViewProps
  2. Add a useEffect block dispatching a new setScalingMode command at the end of the PlayerView function (after and similarly to setFullscreen)
  3. Update the Kotlin and Swift code to handle this new message in RNPlayerViewManager (similar to the setFullscreen method)

@jonathanm-tkf jonathanm-tkf requested a review from krocard October 12, 2023 15:20
@jonathanm-tkf jonathanm-tkf changed the title WIP: initial implementation scaling mode dynamic android implementation scaling mode dynamic android and ios Oct 12, 2023
@jonathanm-tkf
Copy link
Contributor Author

Good morning, @krocard, thanks for your support, regarding the possibility of Dynamic Scaling Mode, we have updated the PR with the implementation for both platforms, I would like the BM team to review the code and any comments are welcome, this would help us a lot if you can generate a new version to remove the temporary fix that we added in our application and not have the need to destroy the player as we have said before.

Copy link
Contributor

@krocard krocard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for the improvements. I'll let @rolandkakonyi review the Swift side.

Copy link
Contributor

@rolandkakonyi rolandkakonyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job @jonathanm-tkf!

Please see my comments.

ios/RNPlayerViewManager.swift Outdated Show resolved Hide resolved
src/components/PlayerView/index.tsx Outdated Show resolved Hide resolved
src/components/PlayerView/index.tsx Outdated Show resolved Hide resolved
@jonathanm-tkf
Copy link
Contributor Author

Thank you very much for the recommendations @krocard @rolandkakonyi

I have confirmed it locally and it works perfectly, again thank you very much for the help.

@rolandkakonyi rolandkakonyi merged commit cc9b11a into bitmovin:development Oct 17, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants